Skip to content

chore(opentelemetry-node): drop instr-redis-4, it is now consolidated into instr-redis #889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 28, 2025
Merged

chore(opentelemetry-node): drop instr-redis-4, it is now consolidated into instr-redis #889

merged 3 commits into from
Jul 28, 2025

Conversation

@trentm
Copy link
Member

@trentm trentm commented Jul 18, 2025

@trentm trentm self-assigned this Jul 18, 2025
@trentm trentm requested review from a team as code owners July 18, 2025 17:45
@github-actions
Copy link

github-actions bot commented Jul 18, 2025 •
edited
Loading

🔍 Preview links for changed docs

@trentm trentm mentioned this pull request Jul 18, 2025
Merged
david-luna
david-luna approved these changes Jul 21, 2025
View reviewed changes
Copy link
Member

@david-luna david-luna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should also update the docs

@theletterf
Copy link
Contributor

theletterf commented Jul 25, 2025

@david-luna Is this a breaking change?

@david-luna
Copy link
Member

david-luna commented Jul 25, 2025

@david-luna Is this a breaking change?

The impact is none if users do not specifically configure redis-4 instrumentation in env vars or via configuration object (which is not documented yet).

As an example if a user is instrumenting a service like this

export OTEL_EXPORTER_OTLP_ENDPOINT="...your-ELASTIC_OTLP_ENDPOINT..."
export OTEL_EXPORTER_OTLP_HEADERS="Authorization=ApiKey ...your-ELASTIC_API_KEY..."
export OTEL_SERVICE_NAME="my-app"
export OTEL_NODE_ENABLED_INSTRUMENTATIONS="http,fastify,pino,redis-4" # reference to the deprecated instrumentation
node --import @elastic/opentelemetry-node my-app.js

if should change to:

export OTEL_EXPORTER_OTLP_ENDPOINT="...your-ELASTIC_OTLP_ENDPOINT..."
export OTEL_EXPORTER_OTLP_HEADERS="Authorization=ApiKey ...your-ELASTIC_API_KEY..."
export OTEL_SERVICE_NAME="my-app"
export OTEL_NODE_ENABLED_INSTRUMENTATIONS="http,fastify,pino,redis" # set to the consolidated one
node --import @elastic/opentelemetry-node my-app.js

If the value is not updated redis package won't be instrumented if the user updates to the version of EDOT containing this change. IMHO is not breaking, just the configuration needs to be tunned but probably important enough to highlight it so users are not surprised about redis traces not being collected.

trentm added a commit to elastic/opentelemetry that referenced this pull request Jul 28, 2025
@trentm
Various updates to EDOT Node.js docs
279f680 
- Update deep links to opentelemetry-js-contrib due to directory moves in that repo.
- Supported technologies table updates:
    - Point to instrumentation-redis rather than instrumentation-redis-4. The former was accidentally not included, and the latter is now rolled into the former (elastic/elastic-otel-node#889).
    - Mention some included instrumentations that were missing from this table: connect, cucumber, dns, runtime-node

Fixes: elastic/elastic-otel-node#869
Refs: elastic/elastic-otel-node#889 (review)
@trentm trentm mentioned this pull request Jul 28, 2025
Merged
@trentm
Copy link
Member Author

trentm commented Jul 28, 2025

elastic/opentelemetry#375 to update the docs.

@theletterf Do you think this is important enough to add an entry for "breaking changes" for this release?

@theletterf
Copy link
Contributor

theletterf commented Jul 28, 2025

@trentm Perhaps under Other if you feel like it's not a Breaking change, but we should add it.

@trentm trentm merged commit c0cfd96 into main Jul 28, 2025
13 checks passed
@trentm trentm deleted the trentm-consolidated-instr-redis branch July 28, 2025 22:17
theletterf added a commit to elastic/opentelemetry that referenced this pull request Jul 29, 2025
@trentm @colleenmcginnis @theletterf
Various updates to EDOT Node.js docs (#375)
4eeacba 
* Various updates to EDOT Node.js docs

- Update deep links to opentelemetry-js-contrib due to directory moves in that repo.
- Supported technologies table updates:
    - Point to instrumentation-redis rather than instrumentation-redis-4. The former was accidentally not included, and the latter is now rolled into the former (elastic/elastic-otel-node#889).
    - Mention some included instrumentations that were missing from this table: connect, cucumber, dns, runtime-node

Fixes: elastic/elastic-otel-node#869
Refs: elastic/elastic-otel-node#889 (review)

* markdown syntax typo fix

Co-authored-by: Colleen McGinnis <[email protected]>

* Update configuration.md

---------

Co-authored-by: Colleen McGinnis <[email protected]>
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@david-luna david-luna david-luna approved these changes

Assignees

@trentm trentm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@trentm @theletterf @david-luna